fix: align CrossJoinExec schema metadata merge with the logical plan#23442
Open
fangchenli wants to merge 3 commits into
Open
fix: align CrossJoinExec schema metadata merge with the logical plan#23442fangchenli wants to merge 3 commits into
fangchenli wants to merge 3 commits into
Conversation
github-actions
Bot
added
sqllogictest
nathanb9
reviewed
Jul 10, 2026
Contributor
There was a problem hiding this comment.
Reviewed the change. The core fix looks right: for JoinType::Inner the shared build_join_schema produces a byte-identical field list to the old hand-rolled code, so only the metadata precedence changes to left-wins, matching the logical plan. Proto roundtrip re-derives the schema via CrossJoinExec::new, so no compat issue there.
A few comments inline, one about a remaining gap worth documenting.
FYI: AI assisted comments - but ive reviewed / verified each 👌
nathanb9
reviewed
Jul 10, 2026
nathanb9
reviewed
Jul 10, 2026
nathanb9
reviewed
Jul 10, 2026
`CrossJoinExec::new` merged schema-level metadata right-biased (`left.metadata().extend(right)`), while the logical plan and the shared physical `build_join_schema` are left-biased for inner joins. When both inputs carry the same metadata key with different values, the cross join's physical schema diverged from the logical one and tripped the physical planner's `schema_satisfied_by` check (raised when the cross join feeds an aggregate), failing with an internal error. Route `CrossJoinExec::new` through the shared `build_join_schema` helper (the one apache#16221 fixed for hash/nested-loop joins but never applied to cross join) so metadata is merged consistently. Field output is unchanged for inner joins. Adds a unit test and a metadata.slt regression over two tables whose schema metadata conflicts under the same key. Closes apache#23434 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fangchenli
force-pushed
the
fix/cross-join-schema-metadata
branch
from
38ec6da to
8bd1e8b
Compare
Add a metadata.slt variant that runs the cross join regression with `join_reordering = false`, so the fix is pinned on the initial (non-swapped) cross join plan and stays insulated from the separate pre-existing swap-path divergence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
A cross join over two inputs whose schemas share a metadata key with different
values produced a physical schema that disagreed with the logical plan, so when
the cross join fed an aggregate the planner failed with "Physical input schema
should be the same as the one converted from logical input schema".
The logical plan and the shared
build_join_schemahelper merge inner-joinmetadata left-biased, but
CrossJoinExec::newmerged its own schema right-biased(
left.metadata().extend(right)). #16221 fixed the shared helper for hash andnested-loop joins but never touched cross join.
What changes are included in this PR?
Route
CrossJoinExec::newthrough the sharedbuild_join_schemahelper so itsmetadata merges the same way as the logical plan. Field output is unchanged for
inner joins.
Known remaining gap (pre-existing, shared with hash/NLJ): under join reordering, the swap path still flips a conflicting schema-level metadata key; tracked in #23461.
Are these changes tested?
Yes, a
metadata.sltregression that cross-joins two tables with conflictingschema metadata into an aggregate. It failed with the internal error before and
passes after.
Are there any user-facing changes?
Bug fix only: affected cross joins now plan and run instead of erroring. No API
changes.